Rewrite IntervalBox to contain an SVector of Intervals #152

dpsanders · 2018-05-17T14:45:07Z

Fixes #150.

Rewrites IntervalBox to contain an SVector.
(Previously IntervalBox was a subtype of StaticVector.
Incorporates #151.

cc @lbenet

coveralls · 2018-05-17T15:07:12Z

Coverage decreased (-0.08%) to 92.165% when pulling 0f8cb49 on rewrite_intervalbox into 777ad9d on master.

coveralls · 2018-05-17T15:07:12Z

Coverage decreased (-0.03%) to 92.218% when pulling ce6bcec on rewrite_intervalbox into 777ad9d on master.

codecov-io · 2018-05-17T15:21:19Z

Codecov Report

Merging #152 into master will decrease coverage by 0.04%.
The diff coverage is 87.5%.

@@            Coverage Diff            @@
##           master    #152      +/-   ##
=========================================
- Coverage   92.24%   92.2%   -0.05%     
=========================================
  Files          21      22       +1     
  Lines        1006    1026      +20     
=========================================
+ Hits          928     946      +18     
- Misses         78      80       +2

Impacted Files	Coverage Δ
src/IntervalArithmetic.jl	`100% <ø> (ø)`	⬆️
src/display.jl	`92.04% <100%> (ø)`	⬆️
src/multidim/setdiff.jl	`100% <100%> (ø)`	⬆️
src/multidim/arithmetic.jl	`83.33% <83.33%> (ø)`
src/multidim/intervalbox.jl	`91.3% <88.23%> (+4.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 777ad9d...ce6bcec. Read the comment docs.

lbenet

Thanks a lot! In mi opinion, this is ready to be merged. I guess this closes #149, since it is not anymore needed, except perhaps for the tests.

lbenet · 2018-05-18T18:52:17Z

src/multidim/arithmetic.jl

+/(a::IntervalBox, b::Real) = IntervalBox( a.v ./ b )
+
+Base.broadcast(f, X::IntervalBox) = IntervalBox(f.(X.v))
+Base.broadcast(f, X::IntervalBox, Y::IntervalBox) = IntervalBox( f.(X.v, Y.v) )


While I truly think that this broadcasting change is very nice, I just noticed that, in order to use a function (say exp) on an IntervalBox, you do require the dot-notation. The following uses the current commit (93bb8e0) of this PR:

julia> A = IntervalBox(1..2, 3..4) [1, 2] × [3, 4] julia> exp.(A) # this is implemented here [2.71828, 7.38906] × [20.0855, 54.5982] julia> exp(A) # throwing an error here is uncomfortable! ERROR: MethodError: no method matching exp(::IntervalArithmetic.IntervalBox{2,Float64}) Closest candidates are: exp(::Float16) at math.jl:950 exp(::Complex{Float16}) at math.jl:951 exp(::BigFloat) at mpfr.jl:517 ...

This is sort of uncomfortable, because you force the user to write whatever functions using the dot notation instead of the usual functions. This is actually not a problem, a priori, but then it should be explicitly written somewhere. My idea with #151 was to avoid this, i.e., use directly exp(A).

Should we allow exp(A) to simply return exp.(A), or shall we leave it as it is now?

Julia has moved to requiring exp.(v) to make explicit that you want to apply exp element-wise, since exp of a vector does not make sense.

I don't understand the context where exp of an IntervalBox makes sense. I don't actually understand your Taylor example.

Julia has moved to requiring exp.(v) to make explicit that you want to apply exp element-wise, since exp of a vector does not make sense.

Fair enough.

lbenet · 2018-05-18T18:53:13Z

src/multidim/intervalbox.jl


-IntervalBox(x::Interval) = IntervalBox( (x,) )  # single interval treated as tuple with one element
-
+Base.getindex(X::IntervalBox, i) = X.v[i]


This replaces what it was in line 11, before; do we need Base.@propagate_inbounds here, or not?

Good question. To be honest I'm not sure, but it probably doesn't hurt.

I'm not sure either... I've never understood why it was needed. Proceed as you prefer.

dpsanders · 2018-05-18T19:20:16Z

Just added it.

dpsanders · 2018-05-18T20:04:26Z

The tests from #151 are included here I believe?

lbenet · 2018-05-18T20:57:34Z

The tests from #151 are included here I believe?

Yes, but using broadcasting, i.e., requiring using the dot notation. See this comment.

lbenet · 2018-05-18T21:53:46Z

The kind of subtleties seems important in what I'm doing in TaylorModels. There, from time to time I need to evaluate expressions of the form f( x0 + v ), where x0 is an IntervalBox, and v is an array with TaylorNs (actually, the independent variables). Using this branch, I get the following errors:

julia> x0
IntervalBox(Interval(0.0, 0.0), Interval(1.0, 1.0))

julia> v
2-element Array{TaylorSeries.TaylorN{IntervalArithmetic.Interval{Float64}},1}:
  Interval(1.0, 1.0) x + 𝒪(‖x‖³)
  Interval(1.0, 1.0) y + 𝒪(‖x‖³)

julia> x0 + v
ERROR: MethodError: no method matching +(::IntervalArithmetic.IntervalBox{2,Float64}, ::Array{TaylorSeries.TaylorN{IntervalArithmetic.Interval{Float64}},1})
...

julia> x0 .+ v
ERROR: MethodError: no method matching +(::IntervalArithmetic.IntervalBox{2,Float64}, ::TaylorSeries.TaylorN{IntervalArithmetic.Interval{Float64}})
...

Can we add methods that at least one of these works?

dpsanders · 2018-05-19T00:11:50Z

I think we would want x0 .+ v to work, and even x0 + v. I also came across similar issues.
But when would exp(x0) be needed?

lbenet · 2018-05-19T00:31:04Z

I'm not sure if we actually need to have f(::IntervalBox), so let's leave that for the time being.

In any case, it would be nice to have, at least, f( x0 .+ v ) working, with x0::IntervalBox and v::AbstractArray. It certainly can be done manually, but I think it is worth having it as a function by itself.

dpsanders · 2018-05-19T03:29:31Z

I still do not understand broadcasting properly, so for now the easiest thing will be to define + for an IntervalBox and a Taylor.

Kolaru · 2018-05-19T17:11:44Z

I think that defining

Base.broadcast(f, X::IntervalBox, v::AbstractVector) = IntervalBox( f.(X, v)... )

does the trick for all number-interval operations, since it is more specific than something like

Base.broadcast(f, A::Any, v::AbstractVector)

which is probably currently called when using this PR with X .+ v, v being a vector.

lbenet · 2018-05-19T18:27:43Z

Good point. Certainly it is worth checking.

lbenet · 2018-05-21T00:20:50Z

I just checked the proposal of @Kolaru , and it does the work. Thanks for the suggestion!

Beside that method, it would be needed as well the one with the arguments exchanged, i.e. Base.broadcast(f, v::AbstractVector, X::IntervalBox).

dpsanders · 2018-05-31T19:01:20Z

Currently things like
mid.(X) and diam.(X) are failing, i.e. things which are not supposed to return an IntervalBox.

(Unfortunately there were no tests for these.)

dpsanders · 2018-05-31T19:40:50Z

The solution seems to be the following: explicitly say how to broadcast those functions:

Base.broadcast(::typeof(mid), X::IntervalBox) = mid.(X.v)

dpsanders · 2018-05-31T20:07:07Z

Other functions:
mig, mag, radius

Also variants of mid with an extra argument.

Kolaru · 2018-06-01T11:10:28Z

Another solution is to define a function which turn vector of Interval into IntervalBox but leaves others unchanged.

choose_type(v::SVector{Interval}) = IntervalBox(v)
choose_type(v) = v

Then broadcasting becomes

Base.broadcast(f, X::IntervalBox) = choose_type(f.(X.v))

and it should work as expected.

dpsanders · 2018-06-01T20:26:38Z

@Kolaru That sounds like a great solution, thanks!

dpsanders · 2018-06-03T15:53:36Z

The failure on 32-bit Windows seems to be a Julia bug: JuliaLang/julia#27402

So I think this is ready to merge.

lbenet · 2018-06-03T18:58:57Z

Merging! Thanks a lot!

Luis Benet and others added 10 commits May 17, 2018 08:54

Arithmetic operations and functions on IntervalBox return IntervalBox

f0bda2a

Add some tests of functions on IntervalBoxes

171a7a9

More tests...

e0114fb

Rewrite IntervalBox to contain an SVector

c3c6439

Modify arithmetic operations for new IntervalBox

f4bf99c

Fix display of IntervalBox

0e4d205

Add missing functions and fix tests

fe31ea8

Rename arithmetic_functions -> arithmetic

770c7d3

Fix broadcasting

aeb6a5b

Fix tests. So far no iteration for IntervalBox

0f8cb49

dpsanders mentioned this pull request May 17, 2018

Add iteration for IntervalBox #153

Closed

dpsanders changed the title ~~Rewrite IntervalBox~~ Rewrite IntervalBox to contain an SVector of Intervals May 17, 2018

dpsanders mentioned this pull request May 17, 2018

Extend evaluate on AbstractArray, instead of Array JuliaDiff/TaylorSeries.jl#167

Merged

lbenet approved these changes May 18, 2018

View reviewed changes

lbenet reviewed May 18, 2018

View reviewed changes

dpsanders added 2 commits May 18, 2018 13:58

Add setindex

d46339c

Add propagate_inbounds

5a62f24

dpsanders added 3 commits May 18, 2018 15:15

Add iteration for IntervalBox

d6cea6e

Add tests for getindex and setindex

818a95e

Typo

93bb8e0

dpsanders added 2 commits June 1, 2018 15:32

Fix broadcasting and add tests

88b2513

Fix broadcasting, take 2

ce6bcec

lbenet merged commit 4657b06 into master Jun 3, 2018

dpsanders deleted the rewrite_intervalbox branch September 25, 2018 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite IntervalBox to contain an SVector of Intervals #152

Rewrite IntervalBox to contain an SVector of Intervals #152

dpsanders commented May 17, 2018 •

edited

Loading

coveralls commented May 17, 2018

coveralls commented May 17, 2018 •

edited

Loading

codecov-io commented May 17, 2018 •

edited

Loading

lbenet left a comment

lbenet May 18, 2018

lbenet May 18, 2018

dpsanders May 19, 2018

lbenet May 19, 2018

lbenet May 18, 2018

dpsanders May 18, 2018

lbenet May 18, 2018

dpsanders commented May 18, 2018

dpsanders commented May 18, 2018

lbenet commented May 18, 2018

lbenet commented May 18, 2018

dpsanders commented May 19, 2018

lbenet commented May 19, 2018

dpsanders commented May 19, 2018

Kolaru commented May 19, 2018 •

edited

Loading

lbenet commented May 19, 2018

lbenet commented May 21, 2018

dpsanders commented May 31, 2018 •

edited

Loading

dpsanders commented May 31, 2018

dpsanders commented May 31, 2018

Kolaru commented Jun 1, 2018

dpsanders commented Jun 1, 2018

dpsanders commented Jun 3, 2018

lbenet commented Jun 3, 2018


		IntervalBox(x::Interval) = IntervalBox( (x,) ) # single interval treated as tuple with one element

		Base.getindex(X::IntervalBox, i) = X.v[i]

Rewrite IntervalBox to contain an SVector of Intervals #152

Rewrite IntervalBox to contain an SVector of Intervals #152

Conversation

dpsanders commented May 17, 2018 • edited Loading

coveralls commented May 17, 2018

coveralls commented May 17, 2018 • edited Loading

codecov-io commented May 17, 2018 • edited Loading

Codecov Report

lbenet left a comment

Choose a reason for hiding this comment

lbenet May 18, 2018

Choose a reason for hiding this comment

lbenet May 18, 2018

Choose a reason for hiding this comment

dpsanders May 19, 2018

Choose a reason for hiding this comment

lbenet May 19, 2018

Choose a reason for hiding this comment

lbenet May 18, 2018

Choose a reason for hiding this comment

dpsanders May 18, 2018

Choose a reason for hiding this comment

lbenet May 18, 2018

Choose a reason for hiding this comment

dpsanders commented May 18, 2018

dpsanders commented May 18, 2018

lbenet commented May 18, 2018

lbenet commented May 18, 2018

dpsanders commented May 19, 2018

lbenet commented May 19, 2018

dpsanders commented May 19, 2018

Kolaru commented May 19, 2018 • edited Loading

lbenet commented May 19, 2018

lbenet commented May 21, 2018

dpsanders commented May 31, 2018 • edited Loading

dpsanders commented May 31, 2018

dpsanders commented May 31, 2018

Kolaru commented Jun 1, 2018

dpsanders commented Jun 1, 2018

dpsanders commented Jun 3, 2018

lbenet commented Jun 3, 2018

dpsanders commented May 17, 2018 •

edited

Loading

coveralls commented May 17, 2018 •

edited

Loading

codecov-io commented May 17, 2018 •

edited

Loading

Kolaru commented May 19, 2018 •

edited

Loading

dpsanders commented May 31, 2018 •

edited

Loading